feat(watch): add staleness auto-close for idle ship tasks - #27
Conversation
When the watcher sees a ship task's window idle past a threshold (default 2h, FM_STALENESS_AUTOCLOSE_SECS), it reclaims the process: landed work gets an ordinary full teardown, unlanded work gets a chat-only teardown that kills the backend, preserves the worktree and uncommitted code on disk, and files a triage bead into the staleness federated store via the new bin/fm-staleness-file.sh helper. The never-discard-unlanded-work guard is unaffected: only the chat process is reclaimed, never the worktree. bin/fm-watch.sh gains STALENESS_AUTOCLOSE_SECS and the FM_TEARDOWN_BIN test seam (matching the existing FM_CREW_STATE_BIN pattern) so the idle backstop can invoke fm-teardown.sh in --staleness-autoclose mode ahead of ordinary stale classification, skipping paused/captain-held and afk-supervised windows. bin/fm-teardown.sh gains --staleness-autoclose <idle-since>, which reuses the existing work_is_landed check to choose between the ordinary full teardown path and the new chat-only path.
…uit before provably-working call
…th retry backoff and evidence"}
…on pane hash change"}
…ecision/blocked gate
…o once per stale hash
…ipt, config vars, state files
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe watcher adds idle staleness auto-close for eligible ship tasks. Teardown preserves unlanded work and files triage evidence. Away-mode reclaim records are reconciled on return. Documentation and tests cover configuration, behavior, retries, and evidence handling. ChangesStaleness auto-close
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 337: Update the automatic-reclaim guidance in AGENTS.md to replace the
deleted state/<id>.status evidence with the surviving staleness bead, preserved
worktree and branch, and state/.staleness-autoclose.log. Keep the surrounding
explanation about quiet endpoints and possible automatic reclaim unchanged.
In `@bin/fm-staleness-file.sh`:
- Around line 43-48: Update the staleness create invocation in the bead_id
assignment to capture stdout only by removing the stderr redirection, while
preserving the existing failure handling and visible stderr output. Keep the
success message based solely on the returned bead ID.
In `@bin/fm-teardown.sh`:
- Around line 1270-1272: Update the conditional around
staleness_chat_only_teardown to handle [ ! -d "$WT" ] separately: report the
missing worktree without claiming it was preserved and skip the triage bead, or
route this case through the ordinary teardown path. Keep the existing chat-only
behavior for worktrees that exist but whose work is not landed.
- Around line 474-479: Update the staleness filing flow around bead_out and the
fm-staleness-file.sh invocation to detect when filing did not produce a
successful bead, including the script’s fail-open success cases. Before removing
the state files, write a durable local record containing the preserved worktree
and project location for any unfiled bead, while keeping reclamation fail-open
and retaining the existing cleanup and success behavior when filing succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ac11422-2353-4ea5-81a3-c9e141073e9e
📒 Files selected for processing (11)
.agents/skills/afk/SKILL.mdAGENTS.mdbin/fm-afk-return.shbin/fm-staleness-file.shbin/fm-teardown.shbin/fm-watch.shdocs/architecture.mddocs/configuration.mddocs/scripts.mdtests/fm-teardown.test.shtests/fm-watch-triage.test.sh
- fm-teardown.sh: write $ID.staleness-unfiled with worktree/branch/project before removing $ID.meta when staleness bead filing fails, so a preserved unlanded worktree never loses its location pointer - fm-teardown.sh: report a missing worktree accurately instead of claiming 'worktree preserved' when $WT does not exist - fm-staleness-file.sh: capture only stdout for $bead_id, leave the 'staleness create' call's stderr unmerged - AGENTS.md: point reclaim evidence at the filed bead / staleness-unfiled fallback, preserved worktree, and .staleness-autoclose.log instead of the deleted $ID.status file
1 participant
Intent
Implement staleness auto-close: when the watcher sees a ship task's window idle past a threshold (default 2h), reclaim the process - landed work gets a full teardown, unlanded work gets a chat-only teardown that preserves the worktree and uncommitted code on disk and files a triage bead into the staleness federated store.
What Changed
bin/fm-watch.sh: detects a ship task whose window has been idle past a configurable threshold, skips it when the crew is provably working or parked at a needs-decision/blocked gate, throttles the provably-working check to once per stale hash, retries reclaim with backoff, and continues to fire during away mode.--staleness-autoclosemode tobin/fm-teardown.shthat gives landed work a full teardown and gives unlanded work a chat-only reclaim that preserves the worktree/branch and files a triage bead into the staleness store; addedbin/fm-staleness-file.shto back the new per-task state files.bin/fm-afk-return.shto surface staleness auto-close evidence on return from away mode, and updatedAGENTS.md,docs/architecture.md,docs/configuration.md,docs/scripts.md, and the afk skill to document the new config variables and state files, with new/expanded coverage intests/fm-teardown.test.shandtests/fm-watch-triage.test.sh.Risk Assessment
✅ Low: This is round 7 of iterative review on the idle>2h staleness auto-close feature; all six prior rounds' findings (ordering vs. provably-working, per-poll cost of the expensive crew-state check both pre- and post-threshold, afk gap, unbounded retry, retry-counter leak across hash changes, and the needs-decision/blocked gate bypass) have been fixed. Tracing the current code (bin/fm-watch.sh's pwf cache keyed by hash, the should_retry backoff gate, and fm-teardown.sh's landed-check/chat-only path) confirms the latest throttle fix is correctly implemented with no new regression, and the guard ordering, afk evidence logging, and retry-exhaustion fallthrough all behave as intended.
Testing
Ran both dedicated shell test suites for the staleness auto-close feature (fm-teardown.test.sh and fm-watch-triage.test.sh) end-to-end against real git worktrees and mocked backend/CLI fixtures; all 87 combined test cases passed (0 failures), directly demonstrating the landed→full-teardown path, the unlanded→chat-only-reclaim-with-preserved-worktree-and-filed-triage-bead path, and every watcher-side gating condition (threshold, provably-working, needs-decision, afk, retry exhaustion) specified in the intent. One local environment quirk (Homebrew's
md5shadowing BSDmd5 -qon PATH) caused an unrelated first run to fail; fixed by prepending/sbinto PATH for the test invocation only (no source change) and reran successfully.Evidence: fm-teardown.test.sh full run output (41/41 passed)
Evidence: fm-watch-triage.test.sh full run output (46/46 passed)
Evidence: Filtered staleness/auto-close test results (8 tests, all passing)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (6) ✅
bin/fm-watch.sh:1020- The idle>2h staleness auto-close check fires as soon as the pane hash is stale (n>=2, not busy) and the last status line isn't paused/captain-held. It runs before the existing stale_is_terminal / pause_state_class("working") / crew_is_provably_working machinery a few lines below (bin/fm-watch.sh:1040-1122), which this codebase built specifically to avoid killing an actively-running pipeline that legitimately sits on a static pane (see the 2026-07 herdr false-surface incident comment at line ~1049: a validating crew's status log goes stale for the whole duration of a no-mistakes validation run, per AGENTS.md's sparse status-reporting contract, so status_is_paused_or_captain_held("$last") won't catch it either). Net effect: a ship task genuinely mid-validation (e.g. a slow test suite or CI wait) with no pane output for >2h will have its live process killed with zero captain notification (the reclaim is deliberately silent), before the existing "provably working" protections ever get a chance to run. Impact is bounded — unlanded work only loses the running process (worktree/branch fully preserved, a triage bead is filed) and landed work is presumably already done — but it does silently abort active-but-quiet work that the rest of this file goes out of its way to protect.🔧 Fix: placeholder - waiting for test results before finishing
1 warning still open:
bin/fm-watch.sh:1027- crew_is_provably_working (which shells out via FM_CREW_STATE_BIN/fm-crew-state.sh and, per its own doc comment at bin/fm-classify-lib.sh:330-333, 'may make a bounded no-mistakes call' with up to a 10s timeout) is checked BEFORE the cheapage_of "$hf" >= STALENESS_AUTOCLOSE_SECStest in the&&chain at bin/fm-watch.sh:1027-1030. Because bash evaluates && left-to-right, this expensive call now runs on EVERY poll cycle (default 15s via FM_POLL) for every ship-task window that is merely idle+non-busy (n>=2), regardless of how far it is from the 2h threshold - not just once the auto-close window is actually reached. This violates the documented contract on crew_absorb_class/crew_is_provably_working ('NOT a pure read... callers run it only on no-verb signal and first-sighting stale paths, never every wake', bin/fm-classify-lib.sh:333). Every other caller in this same file guards the call behind a first-sighting/new-hash check (see bin/fm-watch.sh:1063-1074) specifically to avoid this cost. With multiple ship tasks legitimately idle between turns (a common, non-error state), this adds a serial no-mistakes subprocess call - worth up to 10s each - to every 15s watcher poll, degrading the whole fleet's supervision responsiveness. Swapping the order (cheap age check before the expensive crew_is_provably_working call) fixes it with no behavior change, since the final boolean result of an AND chain is order-independent.🔧 Fix: Reorder staleness auto-close check to short-circuit before provably-working call
2 warnings still open:
bin/fm-watch.sh:1027- Idle>2h auto-close is unconditionally disabled during away mode (! afk_presentguard, bin/fm-watch.sh:1027) with no equivalent reclaim in the away-mode daemon (bin/fm-supervise-daemon.sh only escalates via FM_STALE_ESCALATE_SECS, it doesn't reclaim). The captain still gets a stale-wake escalation via the daemon's one-shot path, but the process is never auto-killed while away - arguably the scenario the feature's own comment ("leaving costly compute idle indefinitely") most needs to cover.bin/fm-watch.sh:1031- A persistently-failing staleness-autoclose reclaim retries every poll forever with no backoff or escalation counter (unlike the wedge-escalation machinery elsewhere in this file). Failures are only recorded in a write-only $STATE/.staleness-autoclose.log that nothing else reads, and the unconditionalcontinuein the trigger block (bin/fm-watch.sh:1031-1032) means the window can never fall through to the ordinary stale/terminal surfacing path, so a stuck task with a broken reclaim has no path to ever notify the captain.🔧 Fix: {"summary": "Run staleness reclaim during afk with retry backoff and evidence"}
1 warning still open:
bin/fm-watch.sh:412- Staleness auto-close retry counters (.staleness-fails-$key / .staleness-next-$key) are only cleared on a successful reclaim (bin/fm-watch.sh:1094, via staleness_autoclose_clear_retries), never when the pane's hash changes (theelsebranch around line 1211-1212 that resets .hash-$key/.count-$key on a new hash does not call it). This contradicts the feature's own design comment, which says a stuck reclaim gives up 'until the pane's hash next changes and resets the counters via staleness_autoclose_clear_retries.' Once a task's retries are exhausted once, staleness auto-close is permanently disabled for that task for the rest of its life, even after it becomes busy again and later goes idle on an entirely new, unrelated stale period. Ordinary stale/wedge surfacing still protects the captain from silence, but the reclaim optimization silently stops working forever for that task. Fix: call staleness_autoclose_clear_retries(key) in the hash-changed branch, or otherwise reset the fail/next-retry files whenever a new hash is observed.🔧 Fix: {"summary": "Clear staleness auto-close retries on pane hash change"}
1 error still open:
bin/fm-watch.sh:1088- The idle>2h auto-close guard only excludes a status log ending in apaused:/captain-held:verb (status_is_paused_or_captain_held, bin/fm-classify-lib.sh:135-141) and a live crew_absorb_class ofworking(crew_is_provably_working). It never checks whether the crew is parked at a captain-relevant gate — aneeds-decision:orblocked:status line (status_is_captain_relevant, bin/fm-classify-lib.sh:100-114), which is exactly what the pre-existing stale_is_terminal path a few lines below (bin/fm-watch.sh:1116) exists to protect and surface, not silently sweep away. Trace: a ship task that hit a no-mistakes ask-user finding appendsneeds-decision: ...and then legitimately sits idle waiting for the captain (per section 7 of AGENTS.md, the worker "processes every synchronous return until completion" only after the captain answers — this can easily take hours, especially now that the reclaim also runs during afk per bin/fm-watch.sh:1084-1087/1109). Once idle>2h: status_is_paused_or_captain_held("needs-decision: ...") is false (its verb list is only paused/captain-held, bin/fm-classify-lib.sh:107-109/135-140), and crew_is_provably_working is false (crew_absorb_class maps run-stepparked/log verbneeds-decisionto neitherworkingnorpaused, so it falls to its finalprintf 'none', bin/fm-classify-lib.sh:334-346). Every guard clause therefore passes and bin/fm-watch.sh:1093 calls staleness_autoclose_reclaim, which (being unlanded mid-pipeline) takes fm-teardown.sh's chat-only path: kills the live backend endpoint and deletes state/<id>.meta/.status (bin/fm-teardown.sh:459-484). The worktree survives, but the interactive process the captain's eventual decision would be sent to is gone and its meta is gone, so the normalsend the same worker one exact decision ... require the matching resolved eventflow (AGENTS.md section 7) can no longer be completed — the decision can only be recovered via the staleness triage bead, defeating the ask-user gate for any task idle past the threshold. This is squarely the scenario the feature's own design comment claims to exclude ("not declared paused or captain-held") but the implementation's verb check is too narrow to actually cover it.🔧 Fix: Skip staleness auto-close when parked at needs-decision/blocked gate
1 warning still open:
bin/fm-watch.sh:1097- crew_is_provably_working is invoked on every ~15s poll cycle (not once per distinct stale hash) for the entire time a ship task stays past the idle>2h threshold and provably working, since neither the retry-budget machinery nor a hash-change gate throttles this call the way the pre-existing terminal-stale path (bin/fm-watch.sh:1136) does. This is the same class of contract violation ('callers run it only on no-verb signal and first-sighting stale paths, never every wake', fm-classify-lib.sh:331-333) that an earlier review round flagged and fixed for the pre-threshold case, recurring here post-threshold for exactly the long-running-validation scenario the feature's own comments describe.🔧 Fix: Throttle post-threshold provably-working check to once per stale hash
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-teardown.test.sh(41/41 passed, exit 0) — includestest_staleness_autoclose_landed_falls_through_to_full_teardownandtest_staleness_autoclose_unlanded_chat_only_preserves_worktree_and_files_bead, which assert the worktree/branch/commit survive on disk and that a bead is filed to the staleness store recording the branchPATH="/sbin:$PATH" bash tests/fm-watch-triage.test.sh(46/46 passed, exit 0) — includes the 6 watcher-side staleness tests: fires past idle>2h threshold and callsfm-teardown.sh --staleness-autoclose; does not fire below threshold; is spared while provably working (e.g. mid no-mistakes validation); is spared and surfaced when parked at a needs-decision gate; also reclaims during afk mode with durable evidence logging; and retries a bounded number of times on persistent reclaim failure before falling through to ordinary stale surfacing✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation